-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #6302 - Treat empty path segments as ambiguous. #6304
Conversation
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
Outdated
Show resolved
Hide resolved
jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
Outdated
Show resolved
Hide resolved
I think we also need to add something in the documentation regarding ambiguous URIs, how to configure and alternatives if they are not exactly correct. @lachlan-roberts do you want to have a go at the doco, or should I push an attempt to this branch? |
@gregw you can push an attempt to this branch if you want. |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
I have moved the doco work to #6312 , so this PR can proceed without waiting for it. |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…, '?' or end of string Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@gregw What do you think about this, should I fix this test to remove the |
@lachlan-roberts Ouch! that's a tough one. There definitely is an empty segment there, but we don't care because it is the last segment and we are happy to ignore empty segments when they are the last ones.... but at the time we check it, we don't know if it is the last one or not. So how do we make |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had enough input into this PR that it needs another reviewer.
However, other than some niggles, it LGTM
jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
I've contributed too much to this. We need another reviewer
jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
Outdated
Show resolved
Hide resolved
jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
_connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986); | ||
assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); | ||
_connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.UNSAFE); | ||
assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add other combinations?
//ambiguous/initial
/ambiguous//middle
/double//ambiguous//
//triple//ambiguous//
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this would be the right place for these extra tests. This is just testing how the compliance modes handle and empty segment.
What combinations cause an empty segment is tested by HttpUriTest
. We already cover these cases, but I have also added these ones exactly to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is at least one negation error in the comments.
Also here are some suggested improvements to the wording of them
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts lachlan@webtide.com